Skip to content

refactor(api_caller): numeric-safety audit of pagination limit casts - #94

Merged
jhamill34 merged 8 commits into
mainfrom
claude/issue-1-phase4-numeric-safety
Aug 27, 2026
Merged

refactor(api_caller): numeric-safety audit of pagination limit casts#94
jhamill34 merged 8 commits into
mainfrom
claude/issue-1-phase4-numeric-safety

Conversation

@jhamill34

Copy link
Copy Markdown
Owner

Summary

Closes #87 (Phase 4 of #1). Stacked on #93 (Phase 3), which is stacked on #91/#90/#89/#88.

api_caller was the only crate allowing clippy::as_conversions/clippy::cast_possible_truncation, pending this dedicated numeric-safety audit rather than a drive-by fix during the earlier lint-hygiene passes.

Enumerating every cast site (with the blanket crate-level allow removed) found the entire fallout is one duplicated block: APICaller::run_internal and AsyncAPICaller::run_internal each independently resolve options["limit"] (a JSON number that may arrive as f64/i64/u64) down to an i32 pagination cap via 3 as-casts apiece (6 sites total, all structurally identical).

  • i64/u64i32 (4 sites): replaced n as i32 with i32::try_from(n).ok(). This is a real correctness fix, not just a lint nag — as truncates via wraparound for int-to-int casts, so a limit of 2^32 previously wrapped to 0, and this code treats total_limit == 0 as "no cap enforced," silently discarding the caller's limit entirely instead of erroring or falling back sanely. It now falls back to DEFAULT_LIMIT on overflow, same as any other unusable/non-numeric limit value.
  • f64i32 (2 sites): kept the as cast, with a narrow #[allow(clippy::cast_possible_truncation, reason = "...")]. Float-to-int as casts have been saturating (not wrapping) since Rust 1.45 — an out-of-range or NaN limit clamps to i32::MAX/i32::MIN/0, and a fractional limit truncates toward zero, both already the intended behavior for a pagination limit.
  • Extracted the duplicated resolver into a new resolve_total_limit(options: &serde_json::Value) -> i32 free function, called from both run_internals. This both removes the literal duplication and avoids a too_many_lines regression the inline fix would otherwise have introduced in AsyncAPICaller::run_internal (it was already at 100/100 lines).
  • Removed the crate-level #[allow(clippy::as_conversions, clippy::cast_possible_truncation)] in lib.rsapi_caller now lints identically to every other crate in the workspace.

No as_conversions sites were found at all: it's restriction-tier and, per Phase 1's lint policy, deliberately not enabled anywhere in this workspace (clippy::restriction isn't meant to be blanket-enabled), so the crate-level allow for it was already redundant before this PR removed it.

Test plan

  • New tests on resolve_total_limit: in-range f64/i64/u64 pass through unchanged; absent/non-numeric falls back to DEFAULT_LIMIT; an out-of-range i64 (i32::MAX + 1) and u64 (u32::MAX + 2) both fall back to DEFAULT_LIMIT instead of wrapping.
  • Verified both new regression tests fail against the original n as i32 casts (confirms they'd have caught this bug) and pass against the fix.
  • cargo build -p api_caller --all-features — clean.
  • cargo clippy -p api_caller --all-features — zero cast_possible_truncation/as_conversions/too_many_lines warnings.
  • cargo test -p api_caller --all-features — 10 tests, 0 failures.
  • cargo clippy --workspace --all-features — zero cast_possible_truncation/as_conversions/too_many_lines warnings anywhere in the repo.
  • cargo build --workspace --all-features / cargo test --workspace --all-features — clean, zero failures.
  • cargo fmt --all -- --check — clean.

Generated by Claude Code

claude added 8 commits August 26, 2026 17:51
…e lint policy

Phase 1 of issue #1. Nearly every crate opened with
`#![warn(clippy::restriction, clippy::pedantic)]` plus a long, drifted
`#![allow(...)]` list — clippy's own docs say `restriction` isn't meant to
be enabled wholesale, and three of the "commonly allowed" lints turned out
to be default-on via `clippy::all` (match_ref_pats, needless_borrowed_reference,
blanket_clippy_restriction_lints), meaning they were suppressing mainstream
clippy output, not opting out of a restriction-tier lint.

- Centralize the policy in root Cargo.toml's new [workspace.lints.clippy]
  table: pedantic stays fully on, each restriction-tier lint was reviewed
  individually and either enabled (ref_patterns, map_err_ignore,
  allow_attributes_without_reason) or explicitly allowed with a documented
  reason (implicit_return, question_mark_used, shadow_reuse/unrelated/same,
  single_call_fn, absolute_paths, mod_module_files, min_ident_chars,
  separated_literal_suffix, std_instead_of_core/alloc,
  arbitrary_source_item_ordering, doc_paragraphs_missing_punctuation).
  too_many_lines/match_ref_pats/needless_borrowed_reference are explicitly
  re-enabled since they were being suppressed despite not being
  restriction-tier.
- Every hand-written crate (14 that had the old block, plus auth/oauth_flow
  and prototypes/workflow_engine which never opted into any lint policy)
  now just does `[lints] workspace = true`.
- Removed service_loader's three `#[cfg(test)] mod test { #![allow(clippy::restriction,
  clippy::pedantic)] }` blocks, now meaningless since the crate root no
  longer blanket-enables either group.
- Fixed the small, mechanical anti-patterns the old suppressions were
  hiding: `extern crate alloc; use alloc::sync::Arc` (etc.) in 13 ordinary
  std crates (zero crates in this workspace are #![no_std]) replaced with
  plain std paths; 4 `.map_err(|_| ...)` sites that discarded the original
  error now capture it in the resulting message.

The ~86-site ref-pattern/match_ref_pats/needless_borrowed_reference rewrite
this now surfaces as warnings, the too_many_lines fallout, and the
api_caller numeric-safety audit (as_conversions/cast_possible_truncation,
kept allowed here with a documented reason) are tracked as follow-ups in
#85, #86, #87 rather than folded into this policy change.

Fixes #1

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Part of #85 (Phase 2 of #1). Converts every &Some(ref x)/match &value {
&Variant(ref x) => ... } site in service_loader, execution_engine, and
service_writer to plain match-ergonomics form (Some(x), Variant(x)) --
purely syntactic, binds the identical reference type as before.

service_writer's clippy::ref_patterns/match_ref_pats/needless_borrowed_reference
warnings are now fully zero; service_loader/execution_engine only retain
pre-existing, out-of-scope warnings (too_many_lines, implicit_clone, etc.)
tracked separately in #86.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Part of #85 (Phase 2 of #1). Converts every &Some(ref x)/match &value {
&Variant(ref x) => ... }/&mut Value::Object(ref mut x) site in api_caller,
filtered_runner, and python_runner to plain match-ergonomics form --
purely syntactic, binds the identical (mutable or immutable) reference
type as before.

api_caller had two duplicated pagination-matching functions (find_results
and the page-size calculator) each repeating the same 4-arm match, plus
duplicated Number-coercion matches in two request-limit resolvers --
fixed all instances in each. filtered_runner and python_runner each had
one &mut/ref mut site not caught by these clippy lints (which don't cover
&mut patterns) but flagged by the same #85 inventory as the same
anti-pattern.

All three crates now have zero clippy::ref_patterns/match_ref_pats/
needless_borrowed_reference warnings.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Closes #85 (Phase 2 of #1). Converts every &Variant(ref x)-style site in
apicli and apid to plain match-ergonomics form -- purely syntactic, binds
the identical reference type as before, with one exception handled
explicitly:

apicli/template.rs's Integer(key) arm bound key: &i64 via ergonomics
where the original &InputTokens::Integer(key) explicit-deref pattern
bound an owned (Copy) i64 -- fixed by dereferencing at the use site
instead of changing the match arm.

apid/main.rs's provide_input handler matches Some((_, tx)) against a
&mut-sourced get_mut() call; ergonomics binds tx: &mut Sender where the
original ref tx bound &Sender, but Sender::send only needs &self so this
is behaviorally inert.

apicli's path.rs and stub.rs account for most of this PR's sites (30 of
33) and weren't flagged by clippy::ref_patterns/match_ref_pats/
needless_borrowed_reference at all -- those lints don't reliably fire on
every &Some(Variant(_))-shaped match arm mixed with `ref`-bound arms in
the same match. Found instead via the original manual site inventory from
#1's scoping research; worth noting since a future clippy-only search of
this codebase would miss them.

apicli/engine.rs's merge() function keeps its outer `match &left`/
`match &right` (left/right are owned Schema values reused later in the
same arms via `one_of.push(left)`/`vec![left, right]`, so the scrutinee
itself can't drop its `&` without moving out from under later use) --
only the arm patterns lost their redundant `&`/`ref`.

apicli/template.rs also fixes the impl ToString for PathKey match's
ref-pattern shape only, not the ToString/Display issue itself (#13).

Confirmed via a full `cargo clippy --workspace --all-features`: zero
ref_patterns/match_ref_pats/needless_borrowed_reference warnings remain
anywhere in the workspace, closing out issue #85.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
…es offenders

Closes #86 (Phase 3 of #1). Phase 1 re-enabled clippy::too_many_lines
workspace-wide; this was the entire remaining fallout -- exactly 2
functions over the 100-line threshold in the whole workspace.

- execution_engine::Engine::run (123 lines): extracted each match arm
  (Swagger/Action/ApiWrapped/SimpleCode) into its own private dispatch_*
  helper method, mirroring the existing dispatch_code_runner/
  resolve_data_connector/resolve_workflow methods already in this impl
  block. run() itself is now just the $input check, identifier parsing,
  the service/manifest lookup, a one-line-per-arm match delegating to the
  new helpers, and the final wrap_result call. dispatch_swagger and
  dispatch_action pick up clippy::too_many_arguments as a result --
  allowed with the same #16 reasoning already used on dispatch_code_runner
  in this file, since each argument is a distinct pass-through dispatch
  input, not a case for a config struct.

- service_loader's handle_schema (122 lines): the oneOf/anyOf/allOf
  composition handling repeated the identical fetch-map-collect shape
  three times. Extracted into a single resolve_schema_list helper
  (parameterized by field name), the read-side mirror of the exact
  dedup already done on the write side in #6 (service_writer's
  handle_composed_schema).

Correction to Phase 1's PR descriptions: binary/apicli/src/engine.rs
(issue #12's target) does NOT trip this lint -- no individual function
there exceeds 100 lines, even though the file itself is oversized overall.
too_many_lines is a per-function metric, so #86 and #12 don't actually
overlap; #12 remains its own separate file-level refactor.

Both changes are pure extract-method refactors with identical logic;
existing tests (16 in execution_engine, 28 in service_loader) all pass
unchanged. Confirmed via a full workspace clippy run: zero too_many_lines
warnings remain anywhere in the repo.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Closes #87 (Phase 4 of #1). Every `as`-cast site in api_caller turned out
to be the same duplicated total_limit resolver (both APICaller::run_internal
and AsyncAPICaller::run_internal): f64/i64/u64 -> i32 for options["limit"].

- i64/u64 -> i32 now use i32::try_from(...).ok(), falling back to
  DEFAULT_LIMIT on overflow instead of silently wrapping (e.g. a u64 limit
  of 2^32 previously wrapped to 0, which this code treats as "no cap
  enforced" - a real correctness bug, not just a lint nag).
- f64 -> i32 keeps the `as` cast with a narrow, reasoned allow: float-to-int
  `as` casts saturate rather than wrap (defined behavior since Rust 1.45),
  so this one was already safe.
- Extracted the duplicated resolver into resolve_total_limit(), which also
  fixed a too_many_lines regression the fix introduced and removed the
  duplication between the two run_internal methods.
- Removed the crate-level #![allow(clippy::as_conversions,
  clippy::cast_possible_truncation)]; api_caller now lints like every
  other crate in the workspace.
- Added regression tests confirming the old wraparound behavior for both
  the i64 and u64 arms (verified failing against the pre-fix casts).
…-too-many-lines

# Conflicts:
#	usecases/execution_engine/src/lib.rs
@jhamill34
jhamill34 changed the base branch from claude/issue-1-phase3-too-many-lines to main August 27, 2026 00:04
@jhamill34
jhamill34 merged commit 5cfa4e2 into main Aug 27, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase 4 of #1: numeric-safety audit of api_caller's as-casts

2 participants